Repository navigation
Conversation
app security check
d42f50b to
b9edbe4
Compare
| dependencies: SecurityCheckPresenterDependencies = defaultDependencies, | ||
| ): Promise<void> { | ||
| if (check.kind === 'cancelled') { | ||
| if (format === 'text') throw new MissingAppConfigurationError(options.directory) |
There was a problem hiding this comment.
Could declining this prompt exit 0 in both text and JSON mode? It currently throws MissingAppConfigurationError in text mode, while JSON reports cancelled and exits 0.
Te convention from docs/cli/json-output.md says declined confirmations exit zero, so the output format shouldn't change the exit status.
There was a problem hiding this comment.
Sure. Was going to do this outside of json work but pulled it here in d6017b7: declining now exits 0 in the terminal too, and shows the same message and next step as information. Without prompts, a missing app configuration still fails, since nothing was declined. It has a patch changeset because the terminal behaviour changes.
| --- | ||
|
|
||
| Report clipboard failures in `app security instructions --copy` and `app security check` as errors instead of crashes. | ||
| Report clipboard failures in `app security instructions --copy` as errors instead of crashes. |
There was a problem hiding this comment.
I think a single changeset for both commands was simpler
There was a problem hiding this comment.
Agreed. #8965's changeset now covers both commands ("Report clipboard failures in app security instructions --copy as errors, and in app security check as warnings, instead of crashing."), and this PR no longer touches it.
b9edbe4 to
d6017b7
Compare
`securityCheck` now returns what it did instead of rendering it: the listed files, the finished scan with its `--blocking` outcome, or a cancellation when the user declines to scan without app configuration. It still records the telemetry. The command passes the outcome to a presenter in security-check/, which renders the gathering warnings and the report, offers and delivers the coding-agent instructions, and sets exit code 1 when `--blocking` is breached. The tip that repeats a prompted selection is still shown before the scan, through a callback that the command passes in. `resolveAppSecuritySelection` returns declining to scan without app configuration as an outcome instead of aborting, so that `--json` can report it. In the terminal the presenter aborts with the same error as before. The text output, prompts and exit codes don't change.
Copying the coding-agent instructions is the last step of `check`, after the scan finished and its report is shown. A clipboard that isn't available aborted the command, so the exit code no longer reported the scan. `check` now warns with the clipboard tool's message and suggests `--print-instructions` instead, and the exit code follows `--blocking`. `instructions --copy` still fails, because copying is all it does.
`--json` prints one result for the scan: the app, the results directory and the two files that the scan replaced, its scope and scan directories, the number of agent checks, and whether `--blocking` passed. A breach still exits 1, after the result is printed. The result holds no findings: `review --json` shows them. Paths are absolute, with native separators. In a terminal the instructions prompt is still offered, on stderr, with choices to include the instructions in the result, and to copy them as well by default. `--print-instructions` includes them without asking. A failed copy warns on stderr and reports `copied: false`. `--list-files` lists the gathered files with absolute paths. Declining to scan without app configuration prints a cancelled result and exits 0. Gathering warnings and the tip to skip the prompts are events on stderr. `check` was the last app security command without typed JSON output, so the exceptions list drops its app security section.
Answering No to "Scan it without app configuration?" is a choice, not a failure. With `--json` it was already a cancelled result with exit 0; in the terminal it aborted with exit 1. It now shows the same message and next step as information, and exits 0 in both formats. Without prompts, a missing app configuration still aborts: nothing was declined.
d6017b7 to
12cc647
Compare
Related to https://github.com/shop/issues-develop/issues/23885
WHY are these changes introduced?
app security checkis the last app security command without--json. Its service also rendered output, prompted and set the exit code partway through, so a JSON mode couldn't be added without separating those.WHAT is this pull request doing?
Adds
--jsontoapp security check. A scan returns:{"status": "success", "valid": true, "app": {…}, "resultsDirectory": "…", "deterministicFindingsPath": "…/deterministic-findings.json", "agentChecksPath": "…/agent-checks.json", "scope": {"includeDirs": [], "excludes": [], "noGitIgnore": false}, "scanDirectories": [{"directory": "…", "origin": "app-directory"}], "agentCheckCount": 36, "blocking": {"level": "none", "blockedCheckCount": 0}, "instructions": {"content": "<Markdown>", "copied": true}}review --jsonis the one place findings are reported.validandblockedCheckCountfollow--blocking, with the same rule and exit codes as today.--json, on stderr. Its choices say that the instructions go into the result.--print-instructionsincludes them without asking. With--skip-instructions, "Nothing", or--no-input,instructionsis left out.--list-files --jsonreturns{status, app, scope, scanDirectories, files: [{path}]}with absolute paths.{"status": "cancelled"}and exits 0. In the terminal it now exits 0 too (commit 4).Commits:
securityCheckreturns alisted,scannedorcancelledoutcome, and a presenter insecurity-check/result.tsrenders it, prompts for instructions and sets the exit code. Declining is an outcome of the selection rather than an error. No behaviour change.checkwarns instead of failing: the scan has finished, so the exit code still follows--blocking. The clipboard changeset in Add typed JSON output toapp security instructions#8965 covers it.--json, and remove the App Security section from the JSON output lint exceptions. The JSON output is covered by Add typed JSON output toapp security clean#8963's changeset.How to manually test your changes?
Build once and run the local CLI directly, so nothing else reaches stdout:
Run these in a real terminal with stdout redirected, so the prompts show while the result goes to the file:
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add